Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: better handling of whitespace #564

Merged
merged 1 commit into from
Jun 15, 2023
Merged

fix: better handling of whitespace #564

merged 1 commit into from
Jun 15, 2023

Conversation

lukekarrys
Copy link
Contributor

No description provided.

@lukekarrys lukekarrys requested a review from a team as a code owner June 15, 2023 04:04
@lukekarrys lukekarrys force-pushed the lk/whitespace branch 3 times, most recently from 0dcb021 to fc08341 Compare June 15, 2023 05:20
@lukekarrys lukekarrys self-assigned this Jun 15, 2023
@lukekarrys lukekarrys merged commit 717534e into main Jun 15, 2023
@lukekarrys lukekarrys deleted the lk/whitespace branch June 15, 2023 19:21
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
@jportner
Copy link

Snyk reported that this PR fixed a vulnerability in semver 7.5.1: https://security.snyk.io/vuln/SNYK-JS-SEMVER-3247795

According to Snyk, it looks like semver 5.7.1 has the same issue. It seems a lot of packages still depend on semver 5.x. Is there any chance of applying this fix in a 5.7.2 release as well?

(I looked but I didn't see a 5.x branch in this repo)

@SymbioticKilla
Copy link

@lukekarrys @wraithgar
It would be awesome to have this fix in 5.x and 6.x branches
Thank you!

@wraithgar
Copy link
Member

At this time, seeing as how v5/6 is a single 1400+ line file with outdated testing deps and no working CI we do not have plans to backport this.

@joaomoreno
Copy link

@wraithgar, while I totally side with you on this, it's much more difficult to bulk upgrade semver to v7 across deep dependency trees of even medium side projects. We have 10+ usages of it just in microsoft/vscode. This is a compliment: semver is very popular and its usage is widespread. We thank you for this library! ❤️

That being said, we like to put our commits where our mouth is. I've backported the fix to v5: https://github.com/npm/node-semver/compare/v5.7.1...joaomoreno:joao/backport-564-to-v5?expand=1. I've also fixed the test suites and added the relevant test cases, all green: https://asciinema.org/a/593286. I hope you can consider this for releasing an eventual 5.7.2 version, given your review. If successful, I would gladly backport the same fix to v6.

I can't quite create a PR, since there's no v5 branch to upstream against. If you'd like to roll this forward, let's create a v5 branch and we'll create a PR from my ref.

@ljharb
Copy link

ljharb commented Jun 26, 2023

That would be amazing; semver v7's dropping of node versions means i'm stuck on v6 for basically ever on dozens of packages.

@1EDExg0ffyXfTEqdIUAYNZGnCeajIxMWd2vaQeP

The babel team has also stated that they'd fork v6 to fix the issue. babel/babel#15720 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants